Migrate editor to use remote backed buffer#10520
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR wires the code editor and file tree to open and save remote-backed buffers, adds conflict notifications, and extends buffer tracking for server-local buffers.
Concerns
- Remote
OpenBufferresponses can still replace the editable buffer unconditionally, which can drop user edits made before the initial load completes. - Local file-tree
CodeSourcestate no longer persists its file path because the only location field is skipped by serde. - Remote save failure handling can clear the dirty state even when the save failed.
- Discarding a remote conflict by closing and reopening the buffer does not guarantee a disk reload when another connection still has that server buffer open.
Found: 1 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // (which contains stale client edits). The subsequent OpenBuffer | ||
| // will create a fresh buffer and read from disk. | ||
| let path_for_close = path_str.clone(); | ||
| client.close_buffer(path_for_close); |
There was a problem hiding this comment.
CloseBuffer does not force a fresh disk read when another connection still has the same server buffer open; the server keeps the stale in-memory buffer and the subsequent OpenBuffer can return it again. Use an explicit reload/accept-server RPC or otherwise force the server to reopen from disk.
There was a problem hiding this comment.
This is a very good call. Updated to have a force_reload flag for opening buffers
moirahuang
left a comment
moirahuang
left a comment
There was a problem hiding this comment.
-
the delay is kinda long when overwriting, can we auto dismiss and then bring back the banner if it fails to save
https://github.com/user-attachments/assets/56e11d37-251f-4329-8c02-0692982b22e6 -
when we discard, we aren't correctly writing the first time. it only correctly writes the 2nd time
https://github.com/user-attachments/assets/221b8d00-f826-48da-8e03-ca39d395637e
|
|
||
| match item { | ||
| FileTreeItem::File { metadata, .. } => { | ||
| // Remote file trees don't support opening files in the editor. |
| let manager = RemoteServerManager::handle(ctx); | ||
| manager.as_ref(ctx).client_for_host(&host_id).cloned() | ||
| }; | ||
| log::debug!( |
There was a problem hiding this comment.
nit: do we need the log debugs here and below?
There was a problem hiding this comment.
I will keep this for now in case of future debugging
| return; | ||
| }; | ||
| if let BufferSource::Remote { sync_clock, .. } = &mut state.source { | ||
| *sync_clock = Some(SyncClock::from_wire(response.server_version, 0)); |
There was a problem hiding this comment.
why do we save this version as 0 vs. the latest version?
moirahuang
left a comment
moirahuang
left a comment
There was a problem hiding this comment.
higher level comment, i think there's some log::debugs that can be cleaned up
| .unwrap_or_else(|| path.clone()); | ||
| self.watcher.update(ctx, |watcher, _ctx| { | ||
| std::mem::drop(watcher.unregister_path(path.as_path())); | ||
| std::mem::drop(watcher.unregister_path(&watch_path)); |
There was a problem hiding this comment.
should we reference count here so if there's other paths to the parent directory we don't unconditionally drop?
| let new_server_version = | ||
| if let BufferSource::ServerLocal { sync_clock, .. } = &mut state.source { | ||
| let sv = sync_clock.bump_server(); | ||
| // Reset client version since the buffer is fresh from disk. |
There was a problem hiding this comment.
i see you added this comment but i'm sorry :woman-bowing: i still don't understand why it's reset vs. being the latest disk version
There was a problem hiding this comment.
This is necessary. The client to server contract is a newly opened buffer starts with client version 0 (which makes sense because it's fresh). If we don't reset here, server and client will disagree about their tracked client version on the next sync
kevinyang372
force-pushed
the
kevin/migrate-editor-to-use-remote-buffer
branch
from
8787b06 to
1b3f6b0
Compare
## Description Wire editor to use remote backed buffer. A few noteable things: 1. LocalCodeEditor (I will change its name in a follow-up) now takes a FileLocation, which could be a local / remote file 2. Existing LocalCodeEditor concepts like conflict resolution banner is now hooked up with the remote logic Also fixed a bug where server detected conflicts aren't being properly forwarded to the client This also removes the remote file tree gating on not opening files ## Testing Tested locally
## Description Wire editor to use remote backed buffer. A few noteable things: 1. LocalCodeEditor (I will change its name in a follow-up) now takes a FileLocation, which could be a local / remote file 2. Existing LocalCodeEditor concepts like conflict resolution banner is now hooked up with the remote logic Also fixed a bug where server detected conflicts aren't being properly forwarded to the client This also removes the remote file tree gating on not opening files ## Testing Tested locally
2 participants
Description
Wire editor to use remote backed buffer. A few noteable things:
Also fixed a bug where server detected conflicts aren't being properly forwarded to the client
This also removes the remote file tree gating on not opening files
Testing
Tested locally